@NaccOll Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@NaccOll Thank you for signing the Contributor License Agreement!

OK. I got an idea. Thank you! However I don't think it has to be that complex internally.

Let's see if we can come up with a setTaskScheduler(TaskScheduler) on the RenewableLockRegistry! With default to nothing. And implement this contract on the RedisLockRegistry. So, if TaskScheduler is provided, we just schedule that renewLock() at a scheduleWithFixedDelay().

I don't think we need anything else like a new type, tracking those threads and so on. Every RedisLock could have that ScheduledFuture for itself to be cancelled on unlock and so.

The implementation of RenewableLockRegistry#renewLock should be a single renewal, which is just a wrapper for the renewal of Lock. But this does not solve the problem of calling timing.

redission generates a task when acquiring the lock and delegates it to ExpirationEntry, and removes the task when unlocking or the lock fails.

The complexity cannot be reduced by scheduleAtFixedRate. It is still necessary to start the scheduled task when acquiring the lock, delegate it to ExpirationEntry, and remove it when unlocking or the lock fails. It just changes the loop task to fixed rate. But I feel that loop task is a better solution, which can ensure that each delayed task is new and orderly. If an error occurs, the worst result is that the lock does not exist when unlocking, which is consistent with the previous result. Using scheduleAtFixedRate, I am not sure whether its scheduling order is as I imagined.

Another point you mentioned is whether a new RedisType is needed. The answer is no. But this is just a stopgap measure. A gradual improvement that does not affect the existing logic is more likely to be accepted without facing "why did you change the design". If you guys want to use RenewableLockRegistry instead of the new RedisType, I can modify the code, but I will still use schedule instead of scheduleAtFixedRate

Pay attention, I said scheduleWithFixedDelay, not Rate.
My point was exactly start this schedule when we lock and scheduler is provided . It will do it trick on background until we cancel it with unlock or so. The beauty of renewal lock contract that api is already there and does not affect anything else.
We may make it opposite : I’ll implement my simple ideas and you review. I just really don’t see reason in such a complexity with those extra internal objects a loops.

@artembilan Could you review my latest commit to confirm whether it complies with the discussion

I have modified most of the problems according to the comments, but I do not intend to implement automatic renewal of JdbcLockRegistry.

There are several reasons for this.

1. I don't think it's a good idea to use Jdbc as a distributed lock. In fact, I think it's a temporary solution with only a database component.

2. The current JdbcLock does not have an automatic expiration mechanism. It only stores a creation time in the database and does not expire. Its expiration should be called by the developer on demand ExpirableLockRegistry#expireUnusedOlderThan, which is different from the key expiration of Redis.

3. Introducing an expiration time in JdbcLock will destroy the original design. It is not known how many people rely on this mechanism that does not expire, and there is a lot of new work involved.

4. Without introducing an expiration time, it is just a simple renewal, and the benefits are unclear. Because people who need renewal often call expireUnusedOlderThan themselves, if a global automatic renewal is introduced, it may also cause their expireUnusedOlderThan to be invalidated.

To implement this function, more scenarios need to be considered, but I can't do this at the moment.

I hope the repository can accept this PR, or quickly implement a similar function. As I mentioned in the comments, a few years ago, a user faced the same problem as me. In fact, I initiated this PR because I was limited by the implementation mechanism of RedisLock. Its implementation is based on enumeration and private. If RedisLockType is an interface instead of an enumeration, I will make a custom implementation in my own project. In fact, my current temporary solution is to implement a RedisLockAdator to achieve similar functions, but it is problematic because it can only complete the creation and cancellation of tasks after the actual lock and unlock and cannot obtain the redis value for renewal comparison. Therefore, when the lock of the same key is acquired alternately by different threads, the renewal task of the second acquisition of the lock may be canceled by the release of the first lock.

@artembilan I have modified the code as per the comments and request re-review.

Merged as 4d08e11.

@NaccOll ,

thank you for contribution; looking forward for more!